-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add proposal for binary management #594
base: main
Are you sure you want to change the base?
Conversation
Not sure if we need to copy this over to here: #593 (comment) Closing that in favor of the PR version. |
packageManager: { | ||
name: 'npm', // names which we would specify and map to individual known projects | ||
version: '', // this would be semantically left open from the spec, but should be considered the equivalent of npm package specifiers | ||
// see: https://github.com/zkat/ssri/blob/latest/README.md#--ssriparsesri-opts%E2%80%94integrity | ||
resolved: { | ||
version: '1.0.0', | ||
uri: 'https://registry.url/path/to/tarball.tgz', | ||
// this would be the output of ssri | ||
digest: '...' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packageManager: { | |
name: 'npm', // names which we would specify and map to individual known projects | |
version: '', // this would be semantically left open from the spec, but should be considered the equivalent of npm package specifiers | |
// see: https://github.com/zkat/ssri/blob/latest/README.md#--ssriparsesri-opts%E2%80%94integrity | |
resolved: { | |
version: '1.0.0', | |
uri: 'https://registry.url/path/to/tarball.tgz', | |
// this would be the output of ssri | |
digest: '...' | |
"packageManager": { | |
"name": "npm", // names which we would specify and map to individual known projects | |
"version": "", // this would be semantically left open from the spec, but should be considered the equivalent of npm package specifiers | |
// see: https://github.com/zkat/ssri/blob/latest/README.md#--ssriparsesri-opts%E2%80%94integrity | |
"resolved": { | |
"version": "1.0.0", | |
"uri": "https://registry.url/path/to/tarball.tgz", | |
// this would be the output of ssri | |
"digest": "sha1.c85a4305534f76d461407b59277b954bac97b5c4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the output from ssri
can be more than just this string right? I didn't want to try to make it look like we were documenting the behavior of that package with this line, but if that is the output I am good with this change as a whole.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
digest
was missing an example. I figured you had uri
with a value and it could be anything so either add examples to all or none.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, I see the dissonance in the example. I guess probably what we should do is take the output from ssri
via a package-lock.json
as an example.
name: 'npm', // names which we would specify and map to individual known projects | ||
version: '', // this would be semantically left open from the spec, but should be considered the equivalent of npm package specifiers | ||
// see: https://github.com/zkat/ssri/blob/latest/README.md#--ssriparsesri-opts%E2%80%94integrity | ||
resolved: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid defining a custom addition to the devEngines
spec? It sounds like what you’re creating here is a lockfile; but there are already lockfiles that accompany package.json
files. I’m not sure we need a standardized lockfile, but if we do, that seems like it should be something proposed on its own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I propose we make on the devEngines
spec. The existing field already had changes requested in that issue which were not addressed, this attempts to address them. Specifically the name is changed to not suggest that the spec requires implementors to implement "download" functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unaware of requests for changes on that spec that haven't been addressed; but if there are, let's deal with notes over there rather than defining a competing alternative.
This is a huge document that feels like something that would come out of a monthslong discovery process. Can we maybe mark this as draft for now and start with a new document that just defines the goals we want to address? Let’s get that written down and hopefully find consensus there, before moving on to various approaches and their pros and cons, etc. One model we could follow is https://github.com/nodejs/loaders/tree/main/doc, where separate files cover prior art, external resources, etc. This PR could be split up into multiple files by topic and then some of them can land without waiting for the goals discussion, such as a doc listing prior art. |
Yeah I agree if is too long/much. And I did mark it as draft after reading this comment the first time yesterday. That said, I am not clear on what we get out of multiple documents. I agree this is not complete, but I think part of the problem that was burning folks out in the previous parts of this conversation was too many separate pr's/issues. As I can totally see it is too long, I am concerned about sharding the conversation again. Would it make you more comfortable if I deleted some parts for now (like some of the solutioning I did) and then we can focus for now on the prior art and goals sections but keep it in one doc? |
One other thing to note which I was just reminded of by a comment in one of this repos issues, the intent is that with this proposal the PMWG takes over the scope of the old version management effort here: https://github.com/nodejs/version-management |
It means that simple parts can just land quickly, like the list of prior art. It's also more useful to have separate files rather than one big one. If you look in the loaders repo for example, we have several design proposals in separate files. That's much easier to work with than everything in one file. You don't need to do one PR per file. Maybe start with one PR for all the history stuff, prior art and links/resources, any other related topics that can be one file per topic. That should be easy to land without much controversy, and we can tackle goals in its own PR. |
Ok, I can work later this week on breaking it up. That said, I would be happy to have comments on what is here before then because largely I have struggled to carve out the time to complete this, so when I get back to it if I can address many things at once it would be more efficient. Also, I don't need to be the person to do this work so if anyone wants to help move this along faster than I am able that would be awesome as well. |
I'm happy to help with the documentation work. My availability is also scattered so it'll be in dribs and drabs over the next few days/weeks. I think the most important thing though is to start collecting a list of goals that either have consensus or majority support. Maybe we could start with the consensus ones and separately add ones that require voting as a later PR. Is the best way to do this via a PR or maybe a GitHub discussion? |
|
||
This has led to a the Node.js project focusing on the well understood (but also highly complicated) aspect of simply supporting one locked version of the toolchain across the many | ||
supported architectures. This is in fact a required area of investment for the project as no matter the local dev toolchain a team or developer chooses, when in prod it is *always* | ||
necessary to support a secure and stable single version workflow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessarily true. I've seen many production deployments use the latest Node 20.x, for example, bumping the minor automatically with each prod deployment. I think all AWS Lambda deployments function like this; I don't think you can choose the minor or patch version for a Lambda deployment. @styfle what is Vercel like in this regard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, Vercel and AWS only support major versions of Node.js. Minor and patch are automatically upgraded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, it could be desirable to select an exact version. I remember a couple times where upgrading Node.js minor broke deployments. In particular when the “type” filed was added to package.json and tooling didn’t know about it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, of course it could be desirable. My point was just that we shouldn't assume that everyone is version pinning.
I think perhaps a better way to put it is that version pinning through lockfiles for code that becomes part of a deployment as a build artifact has become common, but version pinning for the runtime and other development tools has not; or more precisely, that version pinning for development dependencies is often at the semver major level only (Node 20.x). This reflects the importance of getting the latest security updates for a runtime at the expense of stability. That's the tradeoff, and I think the original npm install
took the same approach (pinning only to a minor) but people moved away from it because too many dependencies weren't careful enough to not ship breaking changes in patch releases, but Node is generally careful enough and people aren't tied very closely to it where such pinning is often necessary to avoid breakage; and the cost of missing the latest security updates is high. Package managers are maybe somewhere in between; they really shouldn't need pinning at a level beyond major version; pnpm 8 is an unusual case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused about what we are saying is not true. I said "we need to support" not "everyone must do it this way". My point was just that the debians and docker containers and all that are still required no matter if we ship a version manager to help with local and ci dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused about what we are saying is not true.
This part:
in prod it is always necessary to support a secure and stable single version workflow.
sounds to me like you’re saying that the runtime, if not more, needs to be version-pinned in prod. Maybe you meant “pinned at the semver major level”? I read it as intending to mean pinned exactly.
## The Problem | ||
|
||
A realistic and productive Node.js development environment cannot be setup with *only* tools shipped within the current executables. A version manager for both the `node` | ||
executable and a package manager is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A version manager is certainly not a requirement, either for runtime or for package manager. It's much more common for runtime, but a common workflow is to develop locally using latest Node and test for a specific locked version using Docker or a nonprod hosted environment. That's how I've seen many development teams operate over many years.
In the last decade at work I've seen maybe three dozen or so Node app development teams and none of them have used a package manager version manager. All of those teams have used either npm or a handful used Yarn 1. From what I can tell, package manager version managers are really only popular for pnpm because of its bug in pnpm 8 where the lockfile changes often.
Also this section purports to define the problem but really it's describing a solution: one or more version managers. What problems are those tools solving?
|
||
@TODO list out more about the current version managers and their approaches. | ||
|
||
## A Proposal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This proposal should become its own file, but I would suggest setting it aside until we agree on goals because it might change to address them.
name: 'npm', // names which we would specify and map to individual known projects | ||
version: '', // this would be semantically left open from the spec, but should be considered the equivalent of npm package specifiers | ||
// see: https://github.com/zkat/ssri/blob/latest/README.md#--ssriparsesri-opts%E2%80%94integrity | ||
resolved: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unaware of requests for changes on that spec that haven't been addressed; but if there are, let's deal with notes over there rather than defining a competing alternative.
tool authors into one single direction. If they wanted to limit that `version` cannot be a git repository, that would be acceptable even though `npm` defines that as valid for a | ||
package specifier. | ||
|
||
## Technical Approachs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Technical Approachs | |
## Technical Approaches |
There is an inaccuracy in this under the technical details, or at least something that needs clarification. Most existing version managers do NOT manipulate the nvm-sh points to a shim, meaning a single executable is found in the We tried |
The most popular version manager
I have it under very good authority that this is not the case, maybe you can explain a bit better what you mean here? do you mean the windows version and not what I linked above?
I would love to hear more about this. Maybe you could join up on one of our meetings to share your experience with this? |
I took a quick look at nvm's source and It's worth noting that nvm-sh is the most popular from a Github star perspective, but don't make the mistake of assuming it represents a majority of users or VMs. From the limited data we have, none of the individual VMs have a majority share of the overall user base. Unless I'm mistaken, other VMs (like volta and nodist) use a shim as I described. Tools like n use the symlink approach. There's not too much more to say about switching paths on Windows. Some versions of Windows limit the character count of the PATH. Truncate it and the system becomes volatile. The volatility can be unpredictable. If you append the node path to the end, it may be truncated and node won't be recognized. Prefix it and you risk cutting off system resource routes Windows needs to operate. Newer versions don't have this limitation, but there are a lot of people using older versions of Windows. There is also more than one |
That is who messaged me about your comment :P |
Moved from #593. It should have been a PR but I just was doing that in the collab summit before the session and I didn't have the PMWG repo closed down at the time. Sorry about that.